Skip to content

Conversation

@fanquake
Copy link
Member

This was a good addition to the docs, but the PR was closed. So I've cherry-picked the commit and fixed up Russ's comments as well as the linter issue.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conditional ACK 91cb242, if fixes suggested by jonatack and hebasto are incorporated (good catches!)

[regtest]
rpcport=4000
```
Copy link
Member

@jonatack jonatack Jul 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you decide that comments in the example configuration are a good idea, here is one (entirely optional) version for your consideration:

regtest=1
rpcport=2000          # Non-network specific `rpcport` option
regtest.rpcport=3000  # 1st network specific `rpcport` option -> value chosen by the parser

[regtest]
rpcport=4000          # 2nd network specific `rpcport` option

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like adding comments would make this pretty noisy, however will leave this up to if anyone else thinks they should be added.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this PR has been merged now but I would have liked more comments here as @jonatack suggested. I didn't understand what was going on. I understand there is a trade-off between informing less experienced users and minimizing noise for more experienced users. Perhaps when the minimizing noise option is chosen a Bitcoin StackExchange page can be set up to include those comments. Resources like Jameson Lopp's Bitcoin Core config generator tool are useful too. Though whether you'd want to provide a link to these resources from within a config file or in a comment within the codebase I don't know...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's better to make a new PR at this point; replying on a deeply nested comment on a merged PR isn't going to make much of a difference

Though whether you'd want to provide a link to these resources from within a config file or in a comment within the codebase I don't know...

Would personally prefer to keep it more or less self-contained, external links might change and tend to go stale.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, understood. Thanks

@fanquake fanquake force-pushed the options_bitocin_conf branch from 91cb242 to fa2f991 Compare July 25, 2019 01:58
@fanquake
Copy link
Member Author

Have fixed up all the nits mentioned above.

@laanwj
Copy link
Member

laanwj commented Jul 31, 2019

Thanks a lot for documenting this behavior.
ACK fa2f991

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK fa2f991. Only suggested changes since previous review.

@hebasto
Copy link
Member

hebasto commented Jul 31, 2019

ACK fa2f991, I have reviewed the code and it looks OK, I agree it can be merged.

@jamesob
Copy link
Contributor

jamesob commented Jul 31, 2019

ACK fa2f991

@jonatack
Copy link
Member

ACK fa2f991

pull bot pushed a commit to jaschadub/bitcoin that referenced this pull request Jul 31, 2019
…n.conf

fa2f991 doc: add note on precedence of options in bitcoin.conf (Torkel Rogstad)

Pull request description:

  This was a good addition to the docs, but the PR was closed. So I've cherry-picked the commit and fixed up Russ's comments as well as the linter issue.

ACKs for top commit:
  laanwj:
    ACK fa2f991
  hebasto:
    ACK fa2f991, I have reviewed the code and it looks OK, I agree it can be merged.
  jamesob:
    ACK bitcoin@fa2f991
  jonatack:
    ACK fa2f991
  ryanofsky:
    ACK fa2f991. Only suggested changes since previous review.

Tree-SHA512: d8e7bac19e85ad32205652c3c3036766c611cae52e6e3e8af66a2da054659d914dc153d0cf4ace9c0fa7b41f2a8d74d0edd8d83fe7e984b93d70c01a388cf8ec
@fanquake fanquake merged commit fa2f991 into bitcoin:master Jul 31, 2019
@fanquake fanquake deleted the options_bitocin_conf branch July 31, 2019 23:56
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 1, 2019
…n.conf

fa2f991 doc: add note on precedence of options in bitcoin.conf (Torkel Rogstad)

Pull request description:

  This was a good addition to the docs, but the PR was closed. So I've cherry-picked the commit and fixed up Russ's comments as well as the linter issue.

ACKs for top commit:
  laanwj:
    ACK fa2f991
  hebasto:
    ACK fa2f991, I have reviewed the code and it looks OK, I agree it can be merged.
  jamesob:
    ACK bitcoin@fa2f991
  jonatack:
    ACK fa2f991
  ryanofsky:
    ACK fa2f991. Only suggested changes since previous review.

Tree-SHA512: d8e7bac19e85ad32205652c3c3036766c611cae52e6e3e8af66a2da054659d914dc153d0cf4ace9c0fa7b41f2a8d74d0edd8d83fe7e984b93d70c01a388cf8ec
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 12, 2020
Summary:
Backport of Core [[bitcoin/bitcoin#16448 | PR16448]]

Depends on D7890

Test Plan: Proof-reading in a markdown viewer

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D7891
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 27, 2021
…n.conf

fa2f991 doc: add note on precedence of options in bitcoin.conf (Torkel Rogstad)

Pull request description:

  This was a good addition to the docs, but the PR was closed. So I've cherry-picked the commit and fixed up Russ's comments as well as the linter issue.

ACKs for top commit:
  laanwj:
    ACK fa2f991
  hebasto:
    ACK fa2f991, I have reviewed the code and it looks OK, I agree it can be merged.
  jamesob:
    ACK bitcoin@fa2f991
  jonatack:
    ACK fa2f991
  ryanofsky:
    ACK fa2f991. Only suggested changes since previous review.

Tree-SHA512: d8e7bac19e85ad32205652c3c3036766c611cae52e6e3e8af66a2da054659d914dc153d0cf4ace9c0fa7b41f2a8d74d0edd8d83fe7e984b93d70c01a388cf8ec
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 28, 2021
…n.conf

fa2f991 doc: add note on precedence of options in bitcoin.conf (Torkel Rogstad)

Pull request description:

  This was a good addition to the docs, but the PR was closed. So I've cherry-picked the commit and fixed up Russ's comments as well as the linter issue.

ACKs for top commit:
  laanwj:
    ACK fa2f991
  hebasto:
    ACK fa2f991, I have reviewed the code and it looks OK, I agree it can be merged.
  jamesob:
    ACK bitcoin@fa2f991
  jonatack:
    ACK fa2f991
  ryanofsky:
    ACK fa2f991. Only suggested changes since previous review.

Tree-SHA512: d8e7bac19e85ad32205652c3c3036766c611cae52e6e3e8af66a2da054659d914dc153d0cf4ace9c0fa7b41f2a8d74d0edd8d83fe7e984b93d70c01a388cf8ec
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jun 29, 2021
…n.conf

fa2f991 doc: add note on precedence of options in bitcoin.conf (Torkel Rogstad)

Pull request description:

  This was a good addition to the docs, but the PR was closed. So I've cherry-picked the commit and fixed up Russ's comments as well as the linter issue.

ACKs for top commit:
  laanwj:
    ACK fa2f991
  hebasto:
    ACK fa2f991, I have reviewed the code and it looks OK, I agree it can be merged.
  jamesob:
    ACK bitcoin@fa2f991
  jonatack:
    ACK fa2f991
  ryanofsky:
    ACK fa2f991. Only suggested changes since previous review.

Tree-SHA512: d8e7bac19e85ad32205652c3c3036766c611cae52e6e3e8af66a2da054659d914dc153d0cf4ace9c0fa7b41f2a8d74d0edd8d83fe7e984b93d70c01a388cf8ec
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
…n.conf

fa2f991 doc: add note on precedence of options in bitcoin.conf (Torkel Rogstad)

Pull request description:

  This was a good addition to the docs, but the PR was closed. So I've cherry-picked the commit and fixed up Russ's comments as well as the linter issue.

ACKs for top commit:
  laanwj:
    ACK fa2f991
  hebasto:
    ACK fa2f991, I have reviewed the code and it looks OK, I agree it can be merged.
  jamesob:
    ACK bitcoin@fa2f991
  jonatack:
    ACK fa2f991
  ryanofsky:
    ACK fa2f991. Only suggested changes since previous review.

Tree-SHA512: d8e7bac19e85ad32205652c3c3036766c611cae52e6e3e8af66a2da054659d914dc153d0cf4ace9c0fa7b41f2a8d74d0edd8d83fe7e984b93d70c01a388cf8ec
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 1, 2021
…n.conf

fa2f991 doc: add note on precedence of options in bitcoin.conf (Torkel Rogstad)

Pull request description:

  This was a good addition to the docs, but the PR was closed. So I've cherry-picked the commit and fixed up Russ's comments as well as the linter issue.

ACKs for top commit:
  laanwj:
    ACK fa2f991
  hebasto:
    ACK fa2f991, I have reviewed the code and it looks OK, I agree it can be merged.
  jamesob:
    ACK bitcoin@fa2f991
  jonatack:
    ACK fa2f991
  ryanofsky:
    ACK fa2f991. Only suggested changes since previous review.

Tree-SHA512: d8e7bac19e85ad32205652c3c3036766c611cae52e6e3e8af66a2da054659d914dc153d0cf4ace9c0fa7b41f2a8d74d0edd8d83fe7e984b93d70c01a388cf8ec
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jul 12, 2021
…n.conf

fa2f991 doc: add note on precedence of options in bitcoin.conf (Torkel Rogstad)

Pull request description:

  This was a good addition to the docs, but the PR was closed. So I've cherry-picked the commit and fixed up Russ's comments as well as the linter issue.

ACKs for top commit:
  laanwj:
    ACK fa2f991
  hebasto:
    ACK fa2f991, I have reviewed the code and it looks OK, I agree it can be merged.
  jamesob:
    ACK bitcoin@fa2f991
  jonatack:
    ACK fa2f991
  ryanofsky:
    ACK fa2f991. Only suggested changes since previous review.

Tree-SHA512: d8e7bac19e85ad32205652c3c3036766c611cae52e6e3e8af66a2da054659d914dc153d0cf4ace9c0fa7b41f2a8d74d0edd8d83fe7e984b93d70c01a388cf8ec
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants